Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement thread creation deletion event callback. #506

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

radiantgurl
Copy link
Contributor

No description provided.

@radiantgurl radiantgurl force-pushed the edits branch 2 times, most recently from 4b21fc5 to 39a39d4 Compare December 23, 2024 12:13
@radiantgurl radiantgurl force-pushed the edits branch 3 times, most recently from 494cc56 to 09a55f2 Compare January 4, 2025 02:49
@cheesycod
Copy link

This would be useful

@radiantgurl
Copy link
Contributor Author

This would be useful

I know, and since I felt like others would also want it i decided to open a pull request.

@khvzak
Copy link
Member

khvzak commented Jan 16, 2025

Thanks for the PR, I'll review it soon

@radiantgurl radiantgurl force-pushed the edits branch 4 times, most recently from 40a8676 to e84bce1 Compare January 24, 2025 19:10
Comment on lines +568 to +572
if self.unlikely_memory_error() {
ffi::lua_pushthread(ref_thread)
} else {
protect_lua!(ref_thread, 0, 1, |ref_thread| ffi::lua_pushthread(ref_thread))?
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lua_pushthread does not trigger longjmp

Comment on lines +708 to +710
let main_extra = ExtraData::get(main_state);
let main_raw_lua: &RawLua = (*main_extra).raw_lua();
let _guard = StateGuard::new(main_raw_lua, state);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RawLua is singleton, it never changes (as well as ExtraData instance)

@@ -556,6 +559,21 @@ impl RawLua {
value.push_into_stack(self)
}

pub(crate) unsafe fn push_ref_thread(&self, ref_thread: *mut ffi::lua_State) -> Result<()> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ref_thread seems misleading name here, it cannot be the reference thread (as it's a very special type of thread used internally)

unsafe extern "C-unwind" fn userthread_proc(parent: *mut ffi::lua_State, state: *mut ffi::lua_State) {
callback_error_ext(state, ptr::null_mut(), move |extra, _| {
let raw_lua: &RawLua = (*extra).raw_lua();
let _guard = StateGuard::new(raw_lua, state);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, why we need to switch state here?

let userthread_cb =
mlua_expect!(userthread_cb, "no userthread callback set in userthread_proc");
if parent.is_null() {
raw_lua.push(Value::Nil).unwrap();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems this stack value is never used ?

let _guard = StateGuard::new(main_raw_lua, state);
userthread_cb((*main_extra).lua(), event_info)
} else {
raw_lua.push_ref_thread(parent).unwrap();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

push parent two times?

Comment on lines +87 to +88
/// When a thread is created, it contains the thread that created it.
Created(Thread),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it be vice versa? The thread that was created, not parent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants